Looks like there are a few issues preventing this PR from being merged!

• GitHub Actions are failing:
  • Pre-commit checks

If you'd like me to help, just leave a comment, like

@OpenHands please fix the failing actions on PR #377 at branch `align-default-values-with-evaluation`

Feel free to include any additional details that might help me get this PR into a better state.

_{^{You can manage your notification settings}}

@OpenHands fix precommit issues

I'm on it! simonrosenberg can track my progress at all-hands.dev

I've fixed the pre-commit issues and pushed the changes to the PR branch.

Summary of Changes

The pre-commit checks were failing due to ruff lint errors related to import ordering. The fixes were:

1. benchmarks/commit0/run_infer.py: Removed an unused import (from benchmarks.commit0.config import INFER_DEFAULTS)

2. benchmarks/swebench/run_infer.py: Reordered imports to follow proper alphabetical ordering (moved from benchmarks.swebench.config import INFER_DEFAULTS after the build_images import block)

Verification

• ✅ All pre-commit checks now pass (Ruff format, Ruff lint, PEP8 style check, Pyright type check)
• ✅ Changes are minimal and only address the linting issues
• ✅ Changes have been committed and pushed to the align-default-values-with-evaluation branch

The PR should now pass the pre-commit CI checks.

View full conversation

@OpenHands
Read this PR diff
This PR aligns the default argument values in the benchmarks repository with the values used in the evaluation repository (OpenHands/evaluation).

Then clone the evaluation repository and simplify the code as much as possible. Now that the correct values are defined in the benchmarks repository, those values can be totally removed from the evaluation repository.
Try to simplify and clean the evaluation repo code and make a PR with your changes

I'm on it! simonrosenberg can track my progress at all-hands.dev

@OpenHands
/codereview-roasted
Clone the evaluation repository.
Checkout values in eval-job/values.yaml
The purpose of this PR is to move all those hyperparameters from evaluation repo to the benchmarks repository

Please validate that

1. no hyperparameter has changed values / no behavioral changes are introduced
2. the logic of this PR is sound

I'm on it! simonrosenberg can track my progress at all-hands.dev

Summary

I've completed the task of simplifying the OpenHands/evaluation repository to leverage the default values now defined in the OpenHands/benchmarks repository.

Changes Made

Created PR: https://github.com/OpenHands/evaluation/pull/223 (draft)

Files Modified:

1. eval-job/values.yaml - Removed redundant inference/evaluation defaults:

   • Removed: WORKSPACE_TYPE, CRITIC_NAME, NUM_INFER_WORKERS, MAX_ITERATIONS, MAX_ATTEMPTS, MAX_RETRIES, OUTPUT_DIR
   • Kept: DATASET, DATASET_SPLIT, REPO_SPLIT, DATASET_INFER, DATASET_EVAL (needed for build workflow dispatch)

2. run_swebench.sh - Simplified to only pass run-specific arguments (EVAL_LIMIT, MODEL_SLUG, MODEL_NAME)

3. run_gaia.sh - Simplified to only pass run-specific and GAIA-specific arguments (GAIA_LEVEL, GAIA_SPLIT, EVAL_LIMIT, MODEL_SLUG)

4. run_commit0.sh - Simplified to only pass run-specific arguments

5. run_swtbench.sh - Simplified to only pass run-specific arguments

6. run_swebenchmultimodal.sh - Simplified to only pass run-specific arguments

7. run_eval.sh - Removed benchmark-specific environment variable requirements that are now handled by benchmarks defaults

Benefits

• Reduced duplication: Default values are now defined in one place (benchmarks repo)
• Simpler scripts: Shell scripts are shorter and easier to understand (~70 lines removed)
• Easier maintenance: Changes to defaults only need to be made in the benchmarks repo
• Consistency: Running benchmarks locally uses the same defaults as production

Dependency

This PR depends on OpenHands/benchmarks PR #377 which aligns the default argument values in the benchmarks repository. The evaluation PR should be merged after the benchmarks PR is merged.

View full conversation

Code Review: PR #377 - Align Default Argument Values with Evaluation Repository

Taste Rating: 🟡 Acceptable - Works but has one critical gap

Hyperparameter Validation Summary

I compared the PR's config.py files against the evaluation repository's eval-job/values.yaml. Here's the validation:

✅ Values That Match Correctly

[CRITICAL ISSUES] - Must Fix

🔴 [benchmarks/gaia/config.py] Missing max_retries

GAIA's INFER_DEFAULTS is missing max_retries, but the evaluation repo specifies MAX_RETRIES: "3" for GAIA.

Impact: The args_parser.py no longer has a default for max_retries. When gaia/build_images.py passes args.max_retries (which will be None) to build_all_images(), it will crash with:

assert max_retries >= 1, "max_retries must be at least 1"
TypeError: '>=' not supported between instances of 'NoneType' and 'int'

Fix Required:

# benchmarks/gaia/config.py
INFER_DEFAULTS = {
    "dataset": "gaia-benchmark/GAIA",
    "split": "validation",
    "level": "2023_all",
    "num_workers": 30,
    "max_attempts": 3,
    "max_retries": 3,  # ADD THIS LINE
}

🔴 [Multiple build_images.py files] Build scripts don't set max_retries defaults

The following build scripts use args.max_retries but don't call parser.set_defaults() with max_retries:

• swebench/build_images.py
• swebenchmultimodal/build_images.py
• gaia/build_images.py
• commit0/build_images.py (only sets dataset, split, repo_split)
• swtbench/build_eval_env_images.py (only sets dataset, split)

Impact: All build scripts will crash when max_retries=None is passed.

Fix Options:

1. Add max_retries to each benchmark's INFER_DEFAULTS and use parser.set_defaults(**INFER_DEFAULTS) in build scripts
2. Or restore a sensible default (e.g., 3) in args_parser.py for max_retries

[IMPROVEMENT OPPORTUNITIES]

🟡 [benchmarks/utils/args_parser.py, Line 80-84] Inconsistent None handling

For run_infer.py scripts, max_retries=None is silently converted to 3 by Pydantic's EvalMetadata model default. For build_images.py scripts, it causes a crash. This inconsistency is confusing.

Suggestion: Either restore a global default in args_parser.py, or ensure all config.py files include max_retries.

VERDICT

❌ Needs rework - The GAIA max_retries omission and build script defaults must be addressed before merging.

KEY INSIGHT: The PR correctly aligns inference defaults but overlooks that build scripts also inherit from args_parser.py and need max_retries to be set somewhere.

Logic Soundness Assessment

The overall architecture is sound:

• ✅ Centralizing defaults in config.py per benchmark is a good pattern
• ✅ Using parser.set_defaults() is the correct approach
• ✅ Separating INFER_DEFAULTS and EVAL_DEFAULTS makes sense
• ⚠️ The gap is that build scripts weren't updated to use the new pattern consistently

View full conversation

@juanmichelini commit0 was set to max_retries max_attempts = 1 because commit0 runs are extremely long and it would always rerun 3 times (because back then the critic model finish_with_patch was incorrectly setting everyone to false)
Perhaps we should set both values to 3?

@juanmichelini commit0 was set to max_retries max_attempts = 1 because commit0 runs are extremely long and it would always rerun 3 times (because back then the critic model finish_with_patch was incorrectly setting everyone to false)

Perhaps we should set both values to 3?

max_retries should always be 3 since it's to mitigate infra errors. max_attempts instead can be set to 1

@simonrosenberg why does commit0 only has 8 workers?

@simonrosenberg why does commit0 only has 8 workers?

It was set like this at the start because 16 seemed too much to me. But 16 is actually nothing so it should be bumped to 16